Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document configuring TLS ciphers and log a link to it on raised handshake error #297

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Nov 5, 2024

@consideRatio
Copy link
Member Author

@manics I figured this made sense to include as well, it turns out that people can experience a breaking change with LDAPAuthenticator when they upgrade Python to 3.10 (TLJH 1->2 upgrade) and work against an LDAP server that only accepts older TLS ciphers.

README.md Outdated
# default ciphers accepted with LDAPAuthenticator in Python < 3.10
pre_python310_ciphers = "AES128-SHA:AES256-SHA:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES128-SHA:ECDHE-RSA-AES256-SHA:AES128-GCM-SHA256:AES128-SHA256:AES256-GCM-SHA384:AES256-SHA256:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-SHA256:DHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-CHACHA20-POLY1305:TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256"

# default ciphers accepted with LDAPAuthenticator in Python >= 3.10
Copy link
Member

@manics manics Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may change again in a future Python release. Is there an external reference we can link to instead? If there's one for pre_python310_ciphers that'd be ideal too, otherwise we'll need a list of ciphers every time it changes.

Alternatively we could just link to the relevant GitHub issue #293 here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an external reference we can link to instead?

I wrote this above, I didn't find something better to link to detailing exactly what ciphers are used without running this code snippet:

The default list of ciphers stem from ssl.create_default_context().get_ciphers().


If there's one for pre_python310_ciphers that'd be ideal too, otherwise we'll need a list of ciphers every time it changes.

Yes! There is a variable just above in the example named this :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have btw checked that the ciphers has been the same before and after python 3.10 respectively for versions python 3.7 - 3.13.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was unclear 😄. I know you've got both variables, I was more thinking of how to avoid the README getting infinitely long with information that isn't generally useful and in the worst case misleads people into thinking it's fine to just copy the configuration without understanding why (the ciphers will have been removed from Python for a good reason, and we don't want a lengthy explanation in the README of all the caveats).

How about merge and deal with it later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay yeah - its tricky since the readme is the sole place of documentation currently. I emphasized a lot with the challenge of debugging #293 and the challenge in finding a resolution, so it felt important to have something quite concrete.

In the end the cipher choice is determined by the intersection of accepted ciphers among LDAPAuthenticator and the LDAP server.

I'll do a final look to see if I can manage to make things more robust long term while being very concretely helpful for users with issues and then go for a merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made 56ceff6, I think this shouldn't require maintenance burden long term in any way.

@consideRatio consideRatio merged commit 042ba4e into jupyterhub:main Nov 6, 2024
7 checks passed
@consideRatio
Copy link
Member Author

Thank you @manics for reviewing and helping this move forward!! ❤️ 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLJH upgrade causes SSLV3_ALERT_HANDSHAKE_FAILURE until ciphers explicitly configured
2 participants